Open
Conversation
Backend ======= - Add D_FastLEDAudio.h Add FastLED audio variables to SharedData - WIP - Add Tubes Layout - Add FastLEDAudioDriver - Add FLAudioEffect
WalkthroughAdds I2S-based FastLED audio support (new FastLEDAudioDriver, shared audio fields), a reactive FLAudioEffect, a TubesLayout, driver/effect/layout registrations, FastLED driver update-handler lifecycle fixes, a WaterfallEffect 3D shift refactor, a PlatformIO build bump, and a minor startup serial change. Changes
Sequence Diagram(s)sequenceDiagram
participant I2S as I2S Audio HW
participant AudioDrv as FastLEDAudioDriver
participant Proc as fl::AudioProcessor
participant Shared as SharedData
participant Effect as FLAudioEffect
participant LEDs as FastLED Renderer
I2S->>AudioDrv: deliver audio samples
AudioDrv->>Proc: feed samples
Proc->>Proc: analyze (bands, beat, vocals, percussion)
Proc->>Shared: update bass/mid/treble, beat, vocalsActive, vocalConfidence, percussionType
AudioDrv->>Shared: clear transient beat each loop
Effect->>Shared: read audio fields
alt vocalsActive
Effect->>Effect: render vocal-driven visuals
else beat
Effect->>Effect: trigger beat visuals
else
Effect->>Effect: normal rendering
end
Effect->>LEDs: write pixel output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h (4)
39-42: Unused member variablescurrentBPMandonsetCount.
currentBPMandonsetCountare declared but never read or written anywhere in this class. If they're planned for future use, consider removing them until needed to keep the interface clean (YAGNI).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` around lines 39 - 42, The members currentBPM (float) and onsetCount (uint32_t) in the D_FastLEDAudio class are declared but never used; remove these two unused member declarations from the class in D_FastLEDAudio.h to keep the interface clean (or, if they are intentionally planned, add a clear TODO comment and minimal read/write usage to avoid unused-variable warnings). Locate the declarations named currentBPM and onsetCount in the D_FastLEDAudio class and delete them (or replace with a commented TODO) and rebuild to ensure no references remain.
21-28: Hardcoded I2S pins and static globals in a header file.Two concerns:
Pin configuration: The
#definemacros for I2S pins (6, 4, 5) are hardcoded and will silently shadow or collide with any same-named macros elsewhere. Consider making these configurable (e.g., viaonUpdate/UI controls, as done for other drivers) or at minimum wrapping them in#ifndefguards.Static globals in a header:
i2sConfig,config, andaudioInputarestaticat file scope — each translation unit including this header gets its own copy. If this header is included from multiple.cppfiles, you'll have duplicated state and theaudioInputshared_ptr won't actually be shared. If single-TU inclusion is guaranteed by the build, this works but is fragile.Consider moving the static variables inside the class (as
static inlinemembers or into the.cppfile), and making pin assignments configurable or at least guarded:- `#define` I2S_CLK_PIN 6 - `#define` I2S_WS_PIN 4 - `#define` I2S_SD_PIN 5 - `#define` I2S_CHANNEL fl::Left - -static fl::AudioConfigI2S i2sConfig(I2S_WS_PIN, I2S_SD_PIN, I2S_CLK_PIN, 0, I2S_CHANNEL, 44100, 16, fl::Philips); -static fl::AudioConfig config(i2sConfig); -static fl::shared_ptr<fl::IAudioInput> audioInput;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` around lines 21 - 28, The header hardcodes I2S_PIN macros (I2S_CLK_PIN, I2S_WS_PIN, I2S_SD_PIN) and declares file-scope static objects (i2sConfig, config, audioInput) which cause macro collisions and duplicated state across TUs; change the pin defines to be configurable or at minimum wrap them with `#ifndef` guards (so existing builds can override them) and move i2sConfig, config, and audioInput out of the header into the implementation (or declare them as static inline class members) so there is a single shared instance — update the driver class to accept pin/runtime config (e.g., via onUpdate/UI controls) and instantiate the AudioConfigI2S and shared_ptr<fl::IAudioInput> in the .cpp or as inline static members instead of file-scope statics in the header.
51-83: Callback capture lists are inconsistent — consider uniformity.
onBeat,onVocalStart,onVocalEndcapture[&](by reference), which capturesthisand all locals. These callbacks only accesssharedData(global) and member variables (beatCount,lastBeatTime).onVocalConfidence,onBass,onTrebleuse[](no capture) and only accesssharedData(global).The
[&]captures are safe as long as theFastLEDAudioDriverinstance outlivesaudioProcessor(which it does since it's a member). However, mixing capture styles is slightly inconsistent. TheonVocalStart/onVocalEndlambdas don't need[&]since they only touch the globalsharedData.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` around lines 51 - 83, The callbacks use mixed capture styles; make them explicit and consistent: change audioProcessor.onBeat's lambda to capture [this] (it modifies member variables beatCount and lastBeatTime) and keep audioProcessor.onVocalStart / onVocalEnd as [this] if they will ever touch members (or change them to [] if they only touch the global sharedData), and leave audioProcessor.onVocalConfidence, onBass, onTreble as [] (no capture) since they only use the global sharedData; update the capture lists for the lambdas referenced by onBeat, onVocalStart, onVocalEnd, onVocalConfidence, onBass, and onTreble accordingly so each lambda explicitly captures only what it needs.
94-94: Stop audio input in destructor to release hardware resources.The
audioInputstatic shared_ptr is started insetup()but never stopped. When the driver is destroyed, the I2S hardware resource remains active. CallaudioInput->stop()(designed per FastLED API for this purpose) to properly release the resource:♻️ Proposed fix
- ~FastLEDAudioDriver() override {} + ~FastLEDAudioDriver() override { + if (audioInput) { + audioInput->stop(); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` at line 94, The destructor ~FastLEDAudioDriver() currently does nothing but must stop the shared audioInput resource started in setup(); update the destructor for FastLEDAudioDriver to check the static std::shared_ptr audioInput and call audioInput->stop() (and optionally reset the pointer) to release the I2S hardware resource; locate the static member audioInput and the setup() method for context and ensure the destructor calls audioInput->stop() only if the pointer is non-null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platformio.ini`:
- Around line 207-209: Add the compiler define to force the legacy RMT4 driver:
in the platformio.ini's [moonlight] build_flags (the same place where -D
FASTLED_BUILD is set), append -D FASTLED_RMT5=0 so FastLED uses RMT4 instead of
RMT5; update the build_flags string that currently references FASTLED_BUILD so
it includes this new define to prevent the RMT5 ISR "Handler detached" issues
described.
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h`:
- Around line 47-49: Check the result of fl::IAudioInput::create by verifying
audioInput and errorMsg before calling audioInput->start(); if create fails, log
the descriptive errorMsg (or a fallback message) and avoid calling start so you
don't dereference a null pointer. Add a null guard in the class's loop20ms()
(e.g., if (!audioInput) return;) to skip audio processing when audioInput wasn't
created. Ensure all locations that use audioInput (construction/start and
loop20ms) handle the null case consistently and do not assume successful
creation.
- Around line 86-92: The beat flag is being cleared at the end of loop20ms(), so
any onBeat-set sharedData.beat is erased before FLAudioEffect::loop() can
observe it; move the sharedData.beat = false reset to the start of loop20ms()
(before the while loop that calls audioInput->read() and
audioProcessor.update(sample)) so the flag can be set by the onBeat callback
during processing and persist until the next cycle.
---
Nitpick comments:
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h`:
- Around line 39-42: The members currentBPM (float) and onsetCount (uint32_t) in
the D_FastLEDAudio class are declared but never used; remove these two unused
member declarations from the class in D_FastLEDAudio.h to keep the interface
clean (or, if they are intentionally planned, add a clear TODO comment and
minimal read/write usage to avoid unused-variable warnings). Locate the
declarations named currentBPM and onsetCount in the D_FastLEDAudio class and
delete them (or replace with a commented TODO) and rebuild to ensure no
references remain.
- Around line 21-28: The header hardcodes I2S_PIN macros (I2S_CLK_PIN,
I2S_WS_PIN, I2S_SD_PIN) and declares file-scope static objects (i2sConfig,
config, audioInput) which cause macro collisions and duplicated state across
TUs; change the pin defines to be configurable or at minimum wrap them with
`#ifndef` guards (so existing builds can override them) and move i2sConfig,
config, and audioInput out of the header into the implementation (or declare
them as static inline class members) so there is a single shared instance —
update the driver class to accept pin/runtime config (e.g., via onUpdate/UI
controls) and instantiate the AudioConfigI2S and shared_ptr<fl::IAudioInput> in
the .cpp or as inline static members instead of file-scope statics in the
header.
- Around line 51-83: The callbacks use mixed capture styles; make them explicit
and consistent: change audioProcessor.onBeat's lambda to capture [this] (it
modifies member variables beatCount and lastBeatTime) and keep
audioProcessor.onVocalStart / onVocalEnd as [this] if they will ever touch
members (or change them to [] if they only touch the global sharedData), and
leave audioProcessor.onVocalConfidence, onBass, onTreble as [] (no capture)
since they only use the global sharedData; update the capture lists for the
lambdas referenced by onBeat, onVocalStart, onVocalEnd, onVocalConfidence,
onBass, and onTreble accordingly so each lambda explicitly captures only what it
needs.
- Line 94: The destructor ~FastLEDAudioDriver() currently does nothing but must
stop the shared audioInput resource started in setup(); update the destructor
for FastLEDAudioDriver to check the static std::shared_ptr audioInput and call
audioInput->stop() (and optionally reset the pointer) to release the I2S
hardware resource; locate the static member audioInput and the setup() method
for context and ensure the destructor calls audioInput->stop() only if the
pointer is non-null.
platformio.ini
Outdated
Comment on lines
207
to
209
| -D FASTLED_BUILD=\"20260222\" | ||
| lib_deps = | ||
| https://github.com/FastLED/FastLED#9d0b0eb9b5e59e4093982e0c2bdcfdff72ca80cb ; master 20260221 | ||
| https://github.com/FastLED/FastLED#27c99130c83ab666c4c6dbf389425fec5f27db05 ; master 20260222 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the platformio.ini file around the specified lines and check for build flags
cat -n platformio.ini | head -230 | tail -50Repository: MoonModules/MoonLight
Length of output: 3265
🏁 Script executed:
#!/bin/bash
# Search for all FastLED-related configuration flags in the repo
rg -n "FASTLED_RMT5|FASTLED_RMT4|FASTLED_BUILD|FASTLED_" --type ini --type txt --type h --type cppRepository: MoonModules/MoonLight
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Check the exact commit details and verify build string alignment
# Find the git repository containing the pinned FastLED commit
rg -n "27c99130c83ab666c4c6dbf389425fec5f27db05" --type iniRepository: MoonModules/MoonLight
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for FASTLED_RMT5, FASTLED_RMT4, and related driver flags without file type restrictions
rg -n "FASTLED_RMT5|FASTLED_RMT4|FASTLED_SPI" .Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the complete [moonlight] section and build_flags to see all configuration
sed -n '199,211p' platformio.iniRepository: MoonModules/MoonLight
Length of output: 811
🏁 Script executed:
#!/bin/bash
# Check all build_flags sections across the entire platformio.ini
rg -B2 -A5 "build_flags|FASTLED" platformio.iniRepository: MoonModules/MoonLight
Length of output: 2738
🏁 Script executed:
#!/bin/bash
# Verify the commit date by examining git if available, or search related documentation
# Check if there are any comments/docs about the FastLED version and RMT configuration
rg -n "27c99130c83ab666c4c6dbf389425fec5f27db05|RMT5|RMT4|Handler detached" .Repository: MoonModules/MoonLight
Length of output: 181
🏁 Script executed:
#!/bin/bash
# Check platform-espressif32 version 55.03.37 for ESP-IDF details
# Look for package.json or version info in typical locations
rg -n "esp-idf|ESP-IDF|5\.[0-9]" platformio.ini | head -20Repository: MoonModules/MoonLight
Length of output: 2966
🏁 Script executed:
#!/bin/bash
# Search for any Arduino core version or framework configuration
rg -n "framework|arduino|framework-esp32idf|framework-espidf" platformio.iniRepository: MoonModules/MoonLight
Length of output: 3009
Add -D FASTLED_RMT5=0 to force legacy RMT4 driver and prevent ISR handler degradation.
The FASTLED_BUILD string aligns correctly with the pinned commit. However, platform 55.03.37 uses ESP-IDF 5.5+ with Arduino core 3.x, which causes FastLED to default to the RMT5 driver. RMT5 has documented ISR handler lifecycle issues (see FastLED issue #1903) causing repeated "Handler detached" messages and severe performance drops (~2 FPS). Add -D FASTLED_RMT5=0 to the [moonlight] build_flags to force the stable legacy RMT4 driver.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platformio.ini` around lines 207 - 209, Add the compiler define to force the
legacy RMT4 driver: in the platformio.ini's [moonlight] build_flags (the same
place where -D FASTLED_BUILD is set), append -D FASTLED_RMT5=0 so FastLED uses
RMT4 instead of RMT5; update the build_flags string that currently references
FASTLED_BUILD so it includes this new define to prevent the RMT5 ISR "Handler
detached" issues described.
Collaborator
Author
There was a problem hiding this comment.
Not true on newest FastLED
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
FastLED RMT5 driver fixes February 2026 handler detached
💡 Result:
I couldn’t find any FastLED change/issue (through the public repo + recent searches) specifically tied to a “handler detached” message for the ESP32 RMT5 driver in February 2026—so there doesn’t appear to be a known “Feb 2026” targeted fix labeled that way. (If you have the exact log line + platform/core version, I can search more precisely.)
What is documented/fixed recently around ESP32 RMT5 stability:
- FastLED 3.10.1 fixed an ESP32‑S3 RMT5 break where an internal improvement accidentally set DMA mode true, breaking RMT5. Updating beyond 3.10.1 is recommended if you hit DMA-related failures. [1]
- Ongoing RMT5 reliability discussions (especially Wi‑Fi flicker/jitter) note RMT5’s limits and practical mitigations (e.g., raising RMT interrupt priority to the max the driver allows and increasing buffering), but these are about underruns/flicker rather than a “handler detached” teardown error. [2]
- If you need a robust workaround today, FastLED explicitly recommends switching away from RMT5 in some cases:
- Force legacy RMT4 with
-D FASTLED_RMT5=0(typically requires older ESP32 Arduino core / IDF4-era setups; also note RMT4 and RMT5 can’t coexist). [3] - Or use alternative output paths (I2S/SPI/Adafruit bridge), which users/reporting/dev notes often suggest when RMT5 is unstable under load. [3]
- Force legacy RMT4 with
Most actionable next step: confirm whether your “handler detached” happens only with DMA enabled. If yes, try updating FastLED (>= 3.10.1) first. [1] If it persists, try FASTLED_RMT5=0 (RMT4) or switch to a non-RMT driver as above. [3]
Sources:
[1] FastLED releases (3.10.1 notes: DMA mode accidentally set true, breaking RMT5) https://github.com/fastled/fastled/releases
[2] FastLED issue #2082 (RMT5 + Wi‑Fi flicker; buffering/priority limits/knobs discussion) FastLED/FastLED#2082
[3] FastLED repository documentation (RMT4 vs RMT5 selection; FASTLED_RMT5=0; alternative drivers) https://github.com/FastLED/FastLED
@ewowi, thank you for the correction! I appreciate you letting me know that the RMT5 "Handler detached" issues have been resolved in the newest FastLED version.
Since you originally provided the learning about RMT5 issues (learning ID 9bb17d59-f1a1-4505-b40f-39a68c64be29) and you're now confirming it's no longer applicable with the latest FastLED, would you like me to remove that outdated learning from the knowledge base? This will prevent it from being referenced in future reviews.
✏️ Learnings added
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 130
File: platformio.ini:207-209
Timestamp: 2026-02-22T12:37:03.751Z
Learning: FastLED RMT5 driver issues (ISR handler "Handler detached" messages and performance degradation) have been resolved in FastLED versions as of commit 27c99130c83ab666c4c6dbf389425fec5f27db05 (master 20260222). The `-D FASTLED_RMT5=0` workaround is no longer needed.
<!-- </add_learning>
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-05T13:20:46.001Z
Learning: In MoonLight with FastLED 3.11 on ESP-IDF 5.x+ (Arduino core 3.x): FastLED defaults to RMT5 driver (wraps Espressif's led_strip component), which has known ISR handler lifecycle issues causing repeated "Handler detached" messages every ~500ms and severe performance degradation (e.g., 2 FPS). RMT5 is more prone to Wi-Fi/interrupt jitter than legacy RMT4. Solution: force legacy RMT4 by adding `-D FASTLED_RMT5=0` to build flags in platformio.ini, or switch to FastLED's parallel I/O drivers which don't use RMT. RMT4 and RMT5 cannot coexist in the same binary.
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/platformio.ini : Use PlatformIO to build and upload the ESP32 backend C++ code
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-16T15:53:11.236Z
Learning: For ESP32-PICO-MINI-02 (and similar ESP32 boards with PSRAM), the critical configuration to enable PSRAM is `board_build.arduino.memory_type = qio_qspi` in platformio.ini. The board JSON may already include `-DBOARD_HAS_PSRAM` and `flash_mode: qio`, but without the memory_type setting, PSRAM won't be properly initialized, causing LoadStoreError crashes when code tries to access PSRAM addresses.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-28T18:06:23.868Z
Learning: ESP32-P4 observed lower headroom for httpd task stack in MoonLight: NodeManager rename (create new node + rebuild controls + remove old) can overflow httpd stack when onUpdate runs in httpd. Mitigation: defer only NodeManager rename heavy work to SvelteKit loop via a small pendingRenames queue; keep identity short-circuit in compareRecursive and equality-guarded controls updates. Increasing httpd stack_size for P4 builds is an optional, broader fix.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 121
File: src/MoonLight/Nodes/Drivers/D_FastLED.h:176-176
Timestamp: 2026-02-08T20:43:12.857Z
Learning: In MoonLight FastLED 3.11 driver (src/MoonLight/Nodes/Drivers/D_FastLED.h): Affinity is applied per-channel via fl::ChannelOptions.mAffinity passed to fl::ChannelConfig (not via FastLED.setExclusiveDriver). Keeping setExclusiveDriver commented out leaves all drivers available; FastLED then selects the appropriate driver for each channel based on the mAffinity preference ("RMT", "I2S", "SPI", "PARLIO", or "" for auto). This allows per-channel driver preferences rather than a global restriction.
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/factory_settings.ini : Configure default or factory-set ESP32 parameters in `factory_settings.ini`
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-18T17:07:33.344Z
Learning: FastLED library does not support direct 4-byte-per-pixel (CRGBW) buffers. It always expects CRGB (3-byte) arrays and internally generates the W channel from RGB using .setRgbw() API. Casting a 4-byte RGBW buffer to CRGB* causes misalignment (W becomes next pixel's R). For MoonLight's FastLED driver with channelsPerLight=4: must either (1) repack RGB in-place before FastLED.show() then restore after, (2) use driver-specific channelsPerLight=3 layout, or (3) allocate temporary RGB-only buffer. Option 1 (in-place repacking) is best for ESP32-D0 memory constraints.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-18T12:49:07.297Z
Learning: In MoonLight channel reordering architecture: Keep rgbwBufferMapping in driver task (not effect task) because: (1) Driver has spare CPU cycles waiting for DMA/I2S/Art-Net timing; effect task is CPU-bound and starvation-prone. (2) Contiguous RGB writes in effects are cache-friendly; scattered writes at final offsets are cache-hostile for non-RGB orders (GRB, BGR, etc.). (3) forEachLightIndex already handles sparse mapping iteration; moving reordering to effects adds complexity without benefit since drivers must send full frames anyway (can't skip iteration). (4) rgbwBufferMapping does brightness/gamma LUT + white extraction + reordering—moving just reordering creates code duplication. (5) Effects should express color intent (CRGB), not hardware details (offsetRed/Green/Blue). Driver-side transformation maintains separation of concerns and task load balance for scaling to 98K+ pixels.
Collaborator
Author
There was a problem hiding this comment.
Yes remove it please
There was a problem hiding this comment.
@ewowi, I've removed the outdated learning about the FastLED RMT5 driver issues. The knowledge base now reflects the current state: that RMT5 issues have been resolved in FastLED as of commit 27c99130c83ab666c4c6dbf389425fec5f27db05 (master 20260222), and the -D FASTLED_RMT5=0 workaround is no longer needed. Thank you for keeping the information up to date!
🐰💭
(Learning tidied up!)
/) /)
( •.• )
/>📝✨
⛔ Learnings removed
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-05T13:20:46.001Z
Learning: In MoonLight with FastLED 3.11 on ESP-IDF 5.x+ (Arduino core 3.x): FastLED defaults to RMT5 driver (wraps Espressif's led_strip component), which has known ISR handler lifecycle issues causing repeated "Handler detached" messages every ~500ms and severe performance degradation (e.g., 2 FPS). RMT5 is more prone to Wi-Fi/interrupt jitter than legacy RMT4. Solution: force legacy RMT4 by adding `-D FASTLED_RMT5=0` to build flags in platformio.ini, or switch to FastLED's parallel I/O drivers which don't use RMT. RMT4 and RMT5 cannot coexist in the same binary.
🧠 Learnings used
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-02-05T13:20:46.001Z
Learning: In MoonLight with FastLED 3.11 on ESP-IDF 5.x+ (Arduino core 3.x): FastLED defaults to RMT5 driver (wraps Espressif's led_strip component), which has known ISR handler lifecycle issues causing repeated "Handler detached" messages every ~500ms and severe performance degradation (e.g., 2 FPS). RMT5 is more prone to Wi-Fi/interrupt jitter than legacy RMT4. Solution: force legacy RMT4 by adding `-D FASTLED_RMT5=0` to build flags in platformio.ini, or switch to FastLED's parallel I/O drivers which don't use RMT. RMT4 and RMT5 cannot coexist in the same binary.
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/platformio.ini : Use PlatformIO to build and upload the ESP32 backend C++ code
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-16T15:53:11.236Z
Learning: For ESP32-PICO-MINI-02 (and similar ESP32 boards with PSRAM), the critical configuration to enable PSRAM is `board_build.arduino.memory_type = qio_qspi` in platformio.ini. The board JSON may already include `-DBOARD_HAS_PSRAM` and `flash_mode: qio`, but without the memory_type setting, PSRAM won't be properly initialized, causing LoadStoreError crashes when code tries to access PSRAM addresses.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-28T18:06:23.868Z
Learning: ESP32-P4 observed lower headroom for httpd task stack in MoonLight: NodeManager rename (create new node + rebuild controls + remove old) can overflow httpd stack when onUpdate runs in httpd. Mitigation: defer only NodeManager rename heavy work to SvelteKit loop via a small pendingRenames queue; keep identity short-circuit in compareRecursive and equality-guarded controls updates. Increasing httpd stack_size for P4 builds is an optional, broader fix.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 130
File: platformio.ini:207-209
Timestamp: 2026-02-22T12:37:03.751Z
Learning: FastLED RMT5 driver issues (ISR handler "Handler detached" messages and performance degradation) have been resolved in FastLED versions as of commit 27c99130c83ab666c4c6dbf389425fec5f27db05 (master 20260222). The `-D FASTLED_RMT5=0` workaround is no longer needed.
<!-- </add_learning>
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 121
File: src/MoonLight/Nodes/Drivers/D_FastLED.h:176-176
Timestamp: 2026-02-08T20:43:12.857Z
Learning: In MoonLight FastLED 3.11 driver (src/MoonLight/Nodes/Drivers/D_FastLED.h): Affinity is applied per-channel via fl::ChannelOptions.mAffinity passed to fl::ChannelConfig (not via FastLED.setExclusiveDriver). Keeping setExclusiveDriver commented out leaves all drivers available; FastLED then selects the appropriate driver for each channel based on the mAffinity preference ("RMT", "I2S", "SPI", "PARLIO", or "" for auto). This allows per-channel driver preferences rather than a global restriction.
Learnt from: CR
Repo: MoonModules/MoonLight PR: 0
File: misc/parking/GEMINI.md:0-0
Timestamp: 2025-11-29T16:17:30.619Z
Learning: Applies to misc/parking/**/factory_settings.ini : Configure default or factory-set ESP32 parameters in `factory_settings.ini`
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-18T12:49:07.297Z
Learning: In MoonLight channel reordering architecture: Keep rgbwBufferMapping in driver task (not effect task) because: (1) Driver has spare CPU cycles waiting for DMA/I2S/Art-Net timing; effect task is CPU-bound and starvation-prone. (2) Contiguous RGB writes in effects are cache-friendly; scattered writes at final offsets are cache-hostile for non-RGB orders (GRB, BGR, etc.). (3) forEachLightIndex already handles sparse mapping iteration; moving reordering to effects adds complexity without benefit since drivers must send full frames anyway (can't skip iteration). (4) rgbwBufferMapping does brightness/gamma LUT + white extraction + reordering—moving just reordering creates code duplication. (5) Effects should express color intent (CRGB), not hardware details (offsetRed/Green/Blue). Driver-side transformation maintains separation of concerns and task load balance for scaling to 98K+ pixels.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-18T17:07:33.344Z
Learning: FastLED library does not support direct 4-byte-per-pixel (CRGBW) buffers. It always expects CRGB (3-byte) arrays and internally generates the W channel from RGB using .setRgbw() API. Casting a 4-byte RGBW buffer to CRGB* causes misalignment (W becomes next pixel's R). For MoonLight's FastLED driver with channelsPerLight=4: must either (1) repack RGB in-place before FastLED.show() then restore after, (2) use driver-specific channelsPerLight=3 layout, or (3) allocate temporary RGB-only buffer. Option 1 (in-place repacking) is best for ESP32-D0 memory constraints.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2026-01-06T13:45:03.067Z
Learning: For MoonLight on ESP32 at extreme loads (e.g., 49K channels @ 100 fps): Task watchdog can fire when AppDrivers/AppEffects process many nodes without yielding. Even with vTaskDelay(1) at loop end, the node iteration for-loops (PhysicalLayer::loopDrivers line 108-111, PhysicalLayer::loop line 73-75) can take >5 seconds without yielding, starving IDLE task (priority 0). Solution: add taskYIELD() or vTaskDelay(1) inside the node/layer iteration loops to give IDLE task chances to run and reset watchdog. Lowering task priority won't help since IDLE is already at 0 (lowest).
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 84
File: src/MoonLight/Modules/ModuleLightsControl.h:367-379
Timestamp: 2025-12-28T10:28:24.157Z
Learning: In MoonLight PR `#84`: wrapping effectTask's layerP.loop() with monitorMutex while reading channelsE for monitor emission is thread-safe and prevents partial frames, but introduces serialization between effects rendering and monitor emission. Reading from channelsD (driver/front buffer) instead—with monitorMutex held only during the brief pointer swap—is superior because it maintains full parallelism (effects ║ monitor ║ driver), shows actual LED output, and minimizes mutex hold time.
Learnt from: ewowi
Repo: MoonModules/MoonLight PR: 0
File: :0-0
Timestamp: 2025-12-25T13:34:40.097Z
Learning: For the MoonLight LED system: Effects task can run on Core 0 (PRO_CPU) since it only performs computation (calculating colors, writing to channels array) without timing-critical hardware interaction. WiFi preemption is acceptable for pure computation tasks. Only the driver task (using DMA, I2S, LCD, PARLIO) requires Core 1 (APP_CPU) for uninterrupted timing-critical hardware operations.
There was a problem hiding this comment.
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:#!/bin/bash # First, let's examine the platformio.ini file around the specified lines and check for build flags cat -n platformio.ini | head -230 | tail -50Repository: MoonModules/MoonLight
Length of output: 3265
🏁 Script executed:
#!/bin/bash # Search for all FastLED-related configuration flags in the repo rg -n "FASTLED_RMT5|FASTLED_RMT4|FASTLED_BUILD|FASTLED_" --type ini --type txt --type h --type cppRepository: MoonModules/MoonLight
Length of output: 92
🏁 Script executed:
#!/bin/bash # Check the exact commit details and verify build string alignment # Find the git repository containing the pinned FastLED commit rg -n "27c99130c83ab666c4c6dbf389425fec5f27db05" --type iniRepository: MoonModules/MoonLight
Length of output: 92
🏁 Script executed:
#!/bin/bash # Search for FASTLED_RMT5, FASTLED_RMT4, and related driver flags without file type restrictions rg -n "FASTLED_RMT5|FASTLED_RMT4|FASTLED_SPI" .Repository: MoonModules/MoonLight
Length of output: 47
🏁 Script executed:
#!/bin/bash # Check the complete [moonlight] section and build_flags to see all configuration sed -n '199,211p' platformio.iniRepository: MoonModules/MoonLight
Length of output: 811
🏁 Script executed:
#!/bin/bash # Check all build_flags sections across the entire platformio.ini rg -B2 -A5 "build_flags|FASTLED" platformio.iniRepository: MoonModules/MoonLight
Length of output: 2738
🏁 Script executed:
#!/bin/bash # Verify the commit date by examining git if available, or search related documentation # Check if there are any comments/docs about the FastLED version and RMT configuration rg -n "27c99130c83ab666c4c6dbf389425fec5f27db05|RMT5|RMT4|Handler detached" .Repository: MoonModules/MoonLight
Length of output: 181
🏁 Script executed:
#!/bin/bash # Check platform-espressif32 version 55.03.37 for ESP-IDF details # Look for package.json or version info in typical locations rg -n "esp-idf|ESP-IDF|5\.[0-9]" platformio.ini | head -20Repository: MoonModules/MoonLight
Length of output: 2966
🏁 Script executed:
#!/bin/bash # Search for any Arduino core version or framework configuration rg -n "framework|arduino|framework-esp32idf|framework-espidf" platformio.iniRepository: MoonModules/MoonLight
Length of output: 3009
Add
-D FASTLED_RMT5=0to force legacy RMT4 driver and prevent ISR handler degradation.The FASTLED_BUILD string aligns correctly with the pinned commit. However, platform 55.03.37 uses ESP-IDF 5.5+ with Arduino core 3.x, which causes FastLED to default to the RMT5 driver. RMT5 has documented ISR handler lifecycle issues (see FastLED issue
#1903) causing repeated "Handler detached" messages and severe performance drops (~2 FPS). Add-D FASTLED_RMT5=0to the [moonlight] build_flags to force the stable legacy RMT4 driver.🤖 Prompt for AI Agents
Disabling RMT5 should not be necessary now? The RMT5 driver now allocates more memory during WIFI active mode. Please let me know if this isn't the case. RMT5 will have stress testing either this week if ai credits sustain. Otherwise post friday on refresh.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/MoonBase/Nodes.h (1)
350-356: Optional: prefer0.0fliterals for float member initializers.
float vocalConfidence = 0,float bassLevel = 0, andfloat trebleLevel = 0are valid C++ but using integer literals for float members is less explicit than0.0f.✨ Proposed style fix
- float vocalConfidence = 0; - float bassLevel = 0; - float trebleLevel = 0; + float vocalConfidence = 0.0f; + float bassLevel = 0.0f; + float trebleLevel = 0.0f;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonBase/Nodes.h` around lines 350 - 356, Update the float member initializers to use explicit float literals: change the initializers for vocalConfidence, bassLevel, and trebleLevel from integer zero (e.g. = 0) to 0.0f; keep the boolean members vocalsActive and beat unchanged. This ensures types are explicit for members vocalConfidence, bassLevel, and trebleLevel in the class/struct where they are declared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/MoonBase/Nodes.h`:
- Around line 350-356: Update the float member initializers to use explicit
float literals: change the initializers for vocalConfidence, bassLevel, and
trebleLevel from integer zero (e.g. = 0) to 0.0f; keep the boolean members
vocalsActive and beat unchanged. This ensures types are explicit for members
vocalConfidence, bassLevel, and trebleLevel in the class/struct where they are
declared.
Collaborator
Author
|
@coderabbitai, can you verify this issue FastLED/FastLED#2170 and compare it with the commits in this PR and come with a review summary and propose follow up actions to do in this PR |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🌐 Web query: 💡 Result: FastLED/FastLED Issue Source: [1] 🏁 Script executed: Length of output: 10286
FastLED Issue
|
Collaborator
Author
|
@coderabbitai, regarding
How can I bring these in the FastLEDAudioDriver class: create them in setup() and destroy in the destructor |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2830
Recommended Approach (using pointers)class FastLEDAudioDriver : public Node {
private:
// Member variables for audio configuration
fl::unique_ptr<fl::AudioConfigI2S> i2sConfig;
fl::unique_ptr<fl::AudioConfig> config;
fl::shared_ptr<fl::IAudioInput> audioInput;
public:
static const char* name() { return "FastLED Audio"; }
static uint8_t dim() { return _NoD; }
static const char* tags() { return "☸️"; }
fl::AudioProcessor audioProcessor;
void setup() override {
Node::setup(); // !!
// Create configuration objects
i2sConfig = fl::make_unique<fl::AudioConfigI2S>(
I2S_WS_PIN, I2S_SD_PIN, I2S_CLK_PIN, 0,
I2S_CHANNEL, 44100, 16, fl::Philips);
config = fl::make_unique<fl::AudioConfig>(*i2sConfig);
// Create audio input using the config
fl::string errorMsg;
audioInput = fl::IAudioInput::create(*config, &errorMsg);
if (!audioInput) {
EXT_LOGE(ML_TAG, "Failed to create audio input: %s", errorMsg.c_str());
return;
}
audioInput->start();
// ... rest of your setup code (callbacks, etc.)
}
void loop20ms() override {
if (!audioInput) return;
sharedData.beat = false;
while (fl::AudioSample sample = audioInput->read()) {
audioProcessor.update(sample);
}
}
~FastLEDAudioDriver() override {
if (audioInput) {
audioInput->stop();
}
// unique_ptr will automatically clean up i2sConfig and config
}
};Alternative: If
|
FastLED Audio driver - Move static variables inside class - add controls (gain etc) - add onUpdate to set controls - add readPins to get I2S pins from IO module - add initAudio and stopAudio - destructor FLAudio effect - add Mid
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/MoonLight/Nodes/Effects/E_FastLED.h (1)
48-48:hueis declared but never used.It looks like a copy from
RainbowEffect. Remove it or add a future use to avoid dead state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Effects/E_FastLED.h` at line 48, The field uint16_t hue in E_FastLED.h is unused (leftover from RainbowEffect); either delete the hue declaration to remove dead state or implement its intended use where FastLED effect color cycling is computed (e.g., inside the class/method that updates LED colors in this file) — locate the unused symbol "hue" and either remove it or wire it into the effect update routine so it affects color/hue calculations.src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h (1)
26-27: Optional: replace raw owning pointers withunique_ptr.
i2sConfigandconfigare manual-new/manual-deleteowning pointers. The currentstartAudio()/stopAudio()/destructor pattern is correct, butstd::unique_ptrwould make ownership self-documenting and eliminate the need for the explicit null-checks anddeletecalls instopAudio().♻️ Proposed refactor
- fl::AudioConfigI2S* i2sConfig = nullptr; - fl::AudioConfig* config = nullptr; + std::unique_ptr<fl::AudioConfigI2S> i2sConfig; + std::unique_ptr<fl::AudioConfig> config;void startAudio() { - i2sConfig = new fl::AudioConfigI2S(pinI2SWS, pinI2SSD, pinI2SSCK, 0, - channel == 1 ? fl::Right : channel == 2 ? fl::Both : fl::Left, 44100, 16, fl::Philips); - config = new fl::AudioConfig(*i2sConfig); + i2sConfig = std::make_unique<fl::AudioConfigI2S>(pinI2SWS, pinI2SSD, pinI2SSCK, 0, + channel == 1 ? fl::Right : channel == 2 ? fl::Both : fl::Left, 44100, 16, fl::Philips); + config = std::make_unique<fl::AudioConfig>(*i2sConfig); ... } void stopAudio() { if (audioInput) { audioInput->stop(); audioInput.reset(); } - if (config) { delete config; config = nullptr; } - if (i2sConfig) { delete i2sConfig; i2sConfig = nullptr; } + config.reset(); + i2sConfig.reset(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` around lines 26 - 27, Replace the raw owning pointers i2sConfig and config with std::unique_ptr to make ownership explicit: change their declarations to std::unique_ptr<fl::AudioConfigI2S> i2sConfig and std::unique_ptr<fl::AudioConfig> config, allocate with std::make_unique (or reset) inside startAudio(), and remove explicit delete/null-check logic from stopAudio() and the destructor (use reset() if explicit teardown is needed). Update any pointer usages to dereference the unique_ptr (->) and ensure stopAudio() sets the unique_ptrs to nullptr via reset() if you need to release before scope exit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h`:
- Line 153: The log string passed to EXT_LOGI in the D_FastLEDAudio driver
contains an extra closing parenthesis inside the message "(re)creating
audioInput)"; remove the stray ')' so the call to EXT_LOGI(ML_TAG, "(re)creating
audioInput") prints the correct message. Locate the EXT_LOGI usage in
D_FastLEDAudio (the log call shown) and edit the string literal to drop the
unmatched parenthesis.
- Around line 81-86: Fix the two incorrect log formats in the audio callbacks:
in the audioProcessor.onMid lambda change the log label from "onBass" to "onMid"
so it matches the callback (reference: audioProcessor.onMid and
sharedData.midLevel, ML_TAG), and in the onVocalConfidence log change the printf
format specifier from %d to %f to match the float argument (reference:
audioProcessor.onVocalConfidence). Update those commented log lines accordingly
so they are correct when uncommented.
- Line 51: The lambda passed to moduleIO->addUpdateHandler in FastLEDAudioDriver
captures this and is registered with allowRemove=false causing a permanent
callback to readPins(), risking use-after-free; change the registration to store
the returned handler ID (from addUpdateHandler) as a member (e.g.,
updateHandlerId_), and in the FastLEDAudioDriver destructor call
moduleIO->removeUpdateHandler(updateHandlerId_) (or re-register with
allowRemove=true) to ensure the callback is removed before the object is
destroyed; update the class to hold the handler ID and perform removal in the
destructor (or clearly document framework guarantees if you choose not to
remove).
In `@src/MoonLight/Nodes/Effects/E_FastLED.h`:
- Line 82: Guard against division-by-zero when computing the decrement for
beatLevel: ensure layer->size.y is not zero before using it in 255 /
layer->size.y (for example compute an integer divisor like int divisor =
layer->size.y ? layer->size.y : 1; or use a conditional that substitutes a safe
value) and then use that safe divisor in the MIN expression that updates
beatLevel (referencing beatLevel and layer->size.y in the existing line). This
prevents a hardware exception on Xtensa while preserving the original behavior
when size.y > 0.
- Around line 68-73: The drawLine calls in E_FastLED.h pass columnNr and
columnNr++ in the same expression which causes undefined/unspecified behavior;
fix each call by separating the increment from the function call so x1 and x2
are deterministic (e.g., call layer->drawLine(columnNr, ..., columnNr, ...) and
then increment columnNr on the next line), updating the four drawLine
invocations that use columnNr/columnNr++ to ensure columnNr is only read or
written once per expression.
- Around line 59-79: The drawLine calculations can divide by zero because
maxBass/maxMid/maxTreble/maxVocal may be 0 and the decay can undershoot below 0;
fix by clamping the maxima and decays: before each drawLine that uses e.g.
maxBass, ensure a safeDenom = (maxBass > 0.0001f) ? maxBass : 1.0f (or skip
drawing when both sharedData.bassLevel and maxBass are 0), and replace uses of
"maxX -= 0.01" with "maxX = max(0.0f, maxX - 0.01f)"; apply the same pattern for
maxMid, maxTreble, maxVocal and use those safeDenom variables in the
layer->drawLine calls referenced in this block.
---
Nitpick comments:
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h`:
- Around line 26-27: Replace the raw owning pointers i2sConfig and config with
std::unique_ptr to make ownership explicit: change their declarations to
std::unique_ptr<fl::AudioConfigI2S> i2sConfig and
std::unique_ptr<fl::AudioConfig> config, allocate with std::make_unique (or
reset) inside startAudio(), and remove explicit delete/null-check logic from
stopAudio() and the destructor (use reset() if explicit teardown is needed).
Update any pointer usages to dereference the unique_ptr (->) and ensure
stopAudio() sets the unique_ptrs to nullptr via reset() if you need to release
before scope exit.
In `@src/MoonLight/Nodes/Effects/E_FastLED.h`:
- Line 48: The field uint16_t hue in E_FastLED.h is unused (leftover from
RainbowEffect); either delete the hue declaration to remove dead state or
implement its intended use where FastLED effect color cycling is computed (e.g.,
inside the class/method that updates LED colors in this file) — locate the
unused symbol "hue" and either remove it or wire it into the effect update
routine so it affects color/hue calculations.
Comment on lines
81
to
86
| audioProcessor.onMid([](float level) { | ||
| if (level > 0.01f) { | ||
| sharedData.midLevel = level; | ||
| // EXT_LOGD(ML_TAG, "onBass: %f", level); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Minor: copy-paste log label in onMid callback says "onBass".
Line 84 reads "onBass: %f" inside the onMid lambda — a copy-paste leftover. Also, the onVocalConfidence log (line 71) uses %d for a float argument; should be %f. Both are currently commented out, but worth fixing before uncomment.
🔧 Proposed fix
audioProcessor.onMid([](float level) {
if (level > 0.01f) {
sharedData.midLevel = level;
- // EXT_LOGD(ML_TAG, "onBass: %f", level);
+ // EXT_LOGD(ML_TAG, "onMid: %f", level);
}
}); audioProcessor.onVocalConfidence([](float confidence) {
sharedData.vocalConfidence = sharedData.vocalsActive ? confidence : 0.0;
- // EXT_LOGD(ML_TAG, "onVocalConfidence %d", confidence);
+ // EXT_LOGD(ML_TAG, "onVocalConfidence %f", confidence);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| audioProcessor.onMid([](float level) { | |
| if (level > 0.01f) { | |
| sharedData.midLevel = level; | |
| // EXT_LOGD(ML_TAG, "onBass: %f", level); | |
| } | |
| }); | |
| audioProcessor.onMid([](float level) { | |
| if (level > 0.01f) { | |
| sharedData.midLevel = level; | |
| // EXT_LOGD(ML_TAG, "onMid: %f", level); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` around lines 81 - 86, Fix the
two incorrect log formats in the audio callbacks: in the audioProcessor.onMid
lambda change the log label from "onBass" to "onMid" so it matches the callback
(reference: audioProcessor.onMid and sharedData.midLevel, ML_TAG), and in the
onVocalConfidence log change the printf format specifier from %d to %f to match
the float argument (reference: audioProcessor.onVocalConfidence). Update those
commented log lines accordingly so they are correct when uncommented.
Comment on lines
68
to
73
| uint8_t columnNr = 0; | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.bassLevel / maxBass, CRGB::Red); | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.midLevel / maxMid, CRGB::Orange); | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.trebleLevel / maxTreble, CRGB::Green); | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.vocalConfidence / maxVocal, CRGB::Blue); | ||
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * beatLevel / 255, CRGB::Purple); |
There was a problem hiding this comment.
Critical: columnNr++ as a function argument alongside columnNr is undefined/unspecified behaviour.
Each drawLine call passes columnNr (arg 1, x1) and columnNr++ (arg 3, x2) in the same expression. In C++14 and earlier this is undefined behaviour (unsequenced read and modification). In C++17 it is indeterminately sequenced — the compiler may evaluate arg 3 before arg 1, giving drawLine(1, …, 0, …) instead of the intended drawLine(0, …, 0, …). Since x1 ≠ x2 in that ordering, drawLine renders a diagonal segment rather than a vertical bar, and columnNr itself is left at an unexpected value for subsequent calls.
Separate the increment from the call:
🐛 Proposed fix
- uint8_t columnNr = 0;
- layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.bassLevel / maxBass, CRGB::Red);
- layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.midLevel / maxMid, CRGB::Orange);
- layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.trebleLevel / maxTreble, CRGB::Green);
- layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.vocalConfidence / maxVocal, CRGB::Blue);
- layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * beatLevel / 255, CRGB::Purple);
+ uint8_t col = 0;
+ if (maxBass > 0.0f)
+ layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.bassLevel / maxBass, CRGB::Red);
+ ++col;
+ if (maxMid > 0.0f)
+ layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.midLevel / maxMid, CRGB::Orange);
+ ++col;
+ if (maxTreble > 0.0f)
+ layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.trebleLevel / maxTreble, CRGB::Green);
+ ++col;
+ if (maxVocal > 0.0f)
+ layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.vocalConfidence / maxVocal, CRGB::Blue);
+ ++col;
+ layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * beatLevel / 255, CRGB::Purple);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| uint8_t columnNr = 0; | |
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.bassLevel / maxBass, CRGB::Red); | |
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.midLevel / maxMid, CRGB::Orange); | |
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.trebleLevel / maxTreble, CRGB::Green); | |
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * sharedData.vocalConfidence / maxVocal, CRGB::Blue); | |
| layer->drawLine(columnNr, layer->size.y - 1, columnNr++, layer->size.y - 1 - layer->size.y * beatLevel / 255, CRGB::Purple); | |
| uint8_t col = 0; | |
| if (maxBass > 0.0f) | |
| layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.bassLevel / maxBass, CRGB::Red); | |
| +col; | |
| if (maxMid > 0.0f) | |
| layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.midLevel / maxMid, CRGB::Orange); | |
| +col; | |
| if (maxTreble > 0.0f) | |
| layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.trebleLevel / maxTreble, CRGB::Green); | |
| +col; | |
| if (maxVocal > 0.0f) | |
| layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * sharedData.vocalConfidence / maxVocal, CRGB::Blue); | |
| +col; | |
| layer->drawLine(col, layer->size.y - 1, col, layer->size.y - 1 - layer->size.y * beatLevel / 255, CRGB::Purple); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Nodes/Effects/E_FastLED.h` around lines 68 - 73, The drawLine
calls in E_FastLED.h pass columnNr and columnNr++ in the same expression which
causes undefined/unspecified behavior; fix each call by separating the increment
from the function call so x1 and x2 are deterministic (e.g., call
layer->drawLine(columnNr, ..., columnNr, ...) and then increment columnNr on the
next line), updating the four drawLine invocations that use columnNr/columnNr++
to ensure columnNr is only read or written once per expression.
Collaborator
Author
|
@coderabbitai, can you make an analysis of all the readPins() functions in the repository and come with proposals for improvement as it looks like a lot of similar code is done in all of them. Goal is to minimize the code in specific modules or nodes. The recurring pattern is now: each time local variables for pins are created, checks are done if the state.data["pins"] for these variables have changed and if so stop and start things using these pins. This should be done with minimal code on each place where such a mechanism is needed. This is one of the patterns . ,maybe there are more |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20959
Current State AnalysisFound 5 Common Boilerplate (100% duplicated):
Pattern Variations:
Proposed Solution: PinReader UtilityCreate a declarative, reusable pin reading system: 🏁 Script executed: Length of output: 670 🎯 Proposed Solution:
|
| Benefit | Impact |
|---|---|
| Code reduction | 67-85% fewer lines per implementation |
| Consistency | Same validation & error handling everywhere |
| Maintainability | Fix once, benefit everywhere |
| Readability | Declarative intent vs imperative loops |
| Type safety | Compile-time checks on validators |
| Testing | Test PinReader once vs 5+ implementations |
4. Migration Strategy
Phase 1: Add PinReader utility (this PR or follow-up)
- Create
src/MoonBase/PinReader.h - No breaking changes
Phase 2: Refactor one-by-one
- Start with simplest:
D_Infrared.h - Then:
D_FastLEDAudio.h - Then:
ModuleLightsControl.h,ModuleDrivers.h - Finally: Complex
ModuleIO.h
Phase 3: Extend as needed
- Add support for additional validation types
- Add logging levels as parameters
- Consider async pin updates
5. Alternative: Even Simpler Macro-Based Approach
If C++ lambdas feel too heavy, here's a macro-based alternative:
`#define` READ_PIN_WITH_CHANGE(moduleIO, usage, pinVar, restartFlag) \
do { \
uint8_t __old = pinVar; \
pinVar = PinReader::readSinglePin(moduleIO, usage, GPIO_IS_VALID_GPIO); \
if (pinVar != __old && pinVar != UINT8_MAX) restartFlag = true; \
} while(0)
// Usage:
bool changed = false;
READ_PIN_WITH_CHANGE(moduleIO, pin_I2S_SD, pinI2SSD, changed);
READ_PIN_WITH_CHANGE(moduleIO, pin_I2S_WS, pinI2SWS, changed);
if (changed) restart();🎬 Recommendation
Option 1 (Recommended): Implement the PinReader class-based approach
- More flexible, extensible
- Better for complex scenarios (multiple pins, arrays)
- Easier to test and debug
Option 2: Start with helper functions for common cases
readSinglePin(),readPinArray()only- Simpler, less boilerplate
Would you like me to:
- ✅ Generate the complete
PinReader.himplementation with full error handling? - ✅ Create a refactored version of one file (e.g.,
D_FastLEDAudio.h) as proof-of-concept? - ✅ Plan this as a separate PR with step-by-step migration?
- pio.ini: latest FastLED - SharedData: add percussionType FastLED Audio: - onPercussion: set percussionType, - add updatePin and use in readPins - WIP - destructor: remove UpdateHandler FastLED driver - destruct updateHandlers - FastLED.reset -> FastLED.clear FL Audio effect - Fade control - max values 255 - add beat and percussion - IMU, Infrared driver: destruct updateHandler
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MoonLight/Nodes/Drivers/D_FastLEDDriver.h (1)
65-75:⚠️ Potential issue | 🔴 Critical
allowRemove=falsemakesremoveUpdateHandler(ioUpdateHandler)a silent no-op — use-after-free risk.
StatefulService::removeUpdateHandlererases a handler only when_allowRemove && _id == id. Registering withallowRemove=false(Line 75) means the destructor'sremoveUpdateHandler(ioUpdateHandler)call (Line 357) will never erase the entry. IfmoduleIOoutlives this node, the stale[this]lambda will fire on a destroyed object.
D_FastLEDAudio.hcorrectly passestruefor the same pattern; change Line 75 to match:🛡️ Proposed fix
- false); + true);The same issue applies to
D_IMU.h(Line 41) andD_Infrared.h(Line 108) which also useallowRemove=false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDDriver.h` around lines 65 - 75, The lambda passed to moduleIO->addUpdateHandler is registered with allowRemove=false causing StatefulService::removeUpdateHandler(ioUpdateHandler) in the destructor to be a no-op and creating a use-after-free risk; change the addUpdateHandler call that assigns ioUpdateHandler to pass allowRemove=true (same pattern used in D_FastLEDAudio.h) so the destructor's removeUpdateHandler(ioUpdateHandler) can actually remove the handler and avoid the stale [this] callback; also audit other files noted (D_IMU.h, D_Infrared.h) for the same fix.
♻️ Duplicate comments (3)
src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h (1)
73-73:⚠️ Potential issue | 🟡 Minor
%dformat specifier forfloatconfidencein commented log.This was flagged in a prior review (log format in
onVocalConfidence). The%dshould be%fbefore uncomment.🔧 Proposed fix
- // EXT_LOGD(ML_TAG, "onVocalConfidence %d", confidence); + // EXT_LOGD(ML_TAG, "onVocalConfidence %f", confidence);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` at line 73, The commented-out log in D_FastLEDAudio.h within the onVocalConfidence handler uses the integer format "%d" for the float variable confidence; update the format specifier to "%f" (or a more specific float format like "%.2f") in the log call (EXT_LOGD(ML_TAG, ...)) so that when uncommented it prints the float value correctly.src/MoonLight/Nodes/Drivers/D_Infrared.h (1)
108-108:⚠️ Potential issue | 🔴 Critical
allowRemove=false— same issue asD_FastLEDDriver.handD_IMU.h.
removeUpdateHandlerin the destructor (Line 434) is a no-op because the handler was registered withallowRemove=false.🛡️ Proposed fix
- ioUpdateHandler = moduleIO->addUpdateHandler([this](const String& originId) { readPins(); }, false); + ioUpdateHandler = moduleIO->addUpdateHandler([this](const String& originId) { readPins(); }, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_Infrared.h` at line 108, The ioUpdateHandler is registered with addUpdateHandler(..., false) so removeUpdateHandler becomes a no-op in the destructor; change the registration in D_Infrared (the ioUpdateHandler lambda that calls readPins) to allow removal (pass true or omit the explicit false) so removeUpdateHandler in the class destructor actually unregisters the handler; references: ioUpdateHandler, addUpdateHandler, removeUpdateHandler, readPins.src/MoonLight/Nodes/Drivers/D_IMU.h (1)
41-41:⚠️ Potential issue | 🔴 CriticalSame
allowRemove=falseissue —removeUpdateHandlerin destructor is a silent no-op.Same root cause as
D_FastLEDDriver.hLine 75: registering withallowRemove=falseprevents the entry from being erased byremoveUpdateHandler. Change totrueto make the destructor cleanup functional.🛡️ Proposed fix
- ioUpdateHandler = moduleIO->addUpdateHandler([this](const String& originId) { moduleIO->read([&](ModuleState& state) { i2cActuallyReady = state.data["I2CReady"]; }, name()); }, false); + ioUpdateHandler = moduleIO->addUpdateHandler([this](const String& originId) { moduleIO->read([&](ModuleState& state) { i2cActuallyReady = state.data["I2CReady"]; }, name()); }, true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_IMU.h` at line 41, The update handler is registered with allowRemove=false which prevents removeUpdateHandler from actually removing it in the destructor; update the call site where ioUpdateHandler is assigned (moduleIO->addUpdateHandler([this](const String& originId) { ... }, false)) to pass true for the allowRemove parameter so the handler can be removed by removeUpdateHandler in the destructor (ensure the same symbol names ioUpdateHandler, moduleIO->addUpdateHandler and removeUpdateHandler are used to locate the change).
🧹 Nitpick comments (4)
platformio.ini (1)
204-204: Consider a dedicated audio task rather than inflatingDRIVERS_STACK_SIZE.The comment acknowledges this is a workaround. I2S audio reading and
audioProcessor.update()inloop()adds variable workload to the drivers task alongside LED output, which can interfere with DMA-timing-sensitive operations. A dedicated audio task (lower priority, own stack) would isolate audio processing and letDRIVERS_STACK_SIZErevert to its original 4096 bytes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` at line 204, The DRIVERS_STACK_SIZE was increased to 6144 as a workaround for audio processing in the drivers task; instead create a dedicated audio FreeRTOS task to isolate I2S audio reads and audioProcessor.update() from the drivers/LED work so you can restore DRIVERS_STACK_SIZE to 4096. Implement a new task (e.g., audioTask) with its own stack and lower priority than the drivers/LED task, move the I2S read loop and calls to audioProcessor.update() into that task, and ensure safe data handoff to the drivers (use a thread-safe queue or ring buffer) so DMA/timing-sensitive FastLED operations remain in the original drivers task.src/MoonLight/Nodes/Drivers/D_FastLEDDriver.h (2)
25-26:ioUpdateHandlerandcontrolUpdateHandlershould beprivate.These are internal lifecycle tokens with no reason to be part of the public interface.
♻️ Proposed fix
+private: + update_handler_id_t ioUpdateHandler; + update_handler_id_t controlUpdateHandler; + public: static const char* name() { return "FastLED Driver"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDDriver.h` around lines 25 - 26, The two lifecycle token members ioUpdateHandler and controlUpdateHandler in class D_FastLEDDriver are currently public but should be private; move or relocate update_handler_id_t ioUpdateHandler and update_handler_id_t controlUpdateHandler into the private section of D_FastLEDDriver (or add a private: label above them) so they are not exposed in the public interface, leaving other members and methods unchanged.
66-74:ioUpdateHandlerbody is a stub — I2S pin changes will not trigger channel recreation.The handler only logs but has two TODO comments noting that channels should be deleted and recreated. Until this is implemented, runtime pin reassignments won't apply to the FastLED channel configuration.
Would you like me to open a follow-up issue to implement the channel recreation logic here?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDDriver.h` around lines 66 - 74, The ioUpdateHandler currently only logs and must be implemented to tear down and recreate FastLED channels when I2S pins change: inside the lambda (ioUpdateHandler) delete existing channel objects/resources the same way the class destructor does, then recreate channels and their configs using the same logic as onLayout (respecting layerP.nrOfLedPins and layerP.nrOfAssignedPins / MIN(...) used in the diff) so the new pin assignments take effect; also detect and apply any maxPower changes to per-channel config while recreating. Ensure you use the same helper routines or code paths used by the destructor/onLayout for channel creation and deletion so behavior remains consistent.src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h (1)
199-202: Swap destructor order: remove handler before stopping audio.If
moduleIOfires betweenstopAudio()andremoveUpdateHandler(),readPins()can callstartAudio()on a partially-destroyed object. Removing the handler first closes that window:♻️ Proposed fix
~FastLEDAudioDriver() override { - stopAudio(); moduleIO->removeUpdateHandler(ioUpdateHandler); + stopAudio(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h` around lines 199 - 202, The destructor ~FastLEDAudioDriver currently calls stopAudio() before moduleIO->removeUpdateHandler(ioUpdateHandler), which leaves a race where moduleIO may invoke readPins() and call startAudio() on a partially-destroyed object; fix by removing the update handler first (call moduleIO->removeUpdateHandler(ioUpdateHandler) before stopAudio()) so no IO callbacks can run while shutdown/stopAudio() is executing and ensure ioUpdateHandler and moduleIO references are safe to use during removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/MoonLight/Nodes/Drivers/D_Infrared.h`:
- Line 434: The IRDriver destructor currently lacks the override specifier and
doesn't free RMT/RTOS resources allocated in readPins(); update ~IRDriver() to
be "~IRDriver() override" and, in addition to
moduleIO->removeUpdateHandler(ioUpdateHandler), check if rx_channel and
receive_queue are valid and properly tear them down (call the appropriate RMT
uninstall/stop API for rx_channel and delete the FreeRTOS queue for
receive_queue), then null out those members; ensure you reference the same
member names (rx_channel, receive_queue, ioUpdateHandler) and the allocation
site readPins() so the cleanup mirrors allocation.
---
Outside diff comments:
In `@src/MoonLight/Nodes/Drivers/D_FastLEDDriver.h`:
- Around line 65-75: The lambda passed to moduleIO->addUpdateHandler is
registered with allowRemove=false causing
StatefulService::removeUpdateHandler(ioUpdateHandler) in the destructor to be a
no-op and creating a use-after-free risk; change the addUpdateHandler call that
assigns ioUpdateHandler to pass allowRemove=true (same pattern used in
D_FastLEDAudio.h) so the destructor's removeUpdateHandler(ioUpdateHandler) can
actually remove the handler and avoid the stale [this] callback; also audit
other files noted (D_IMU.h, D_Infrared.h) for the same fix.
---
Duplicate comments:
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h`:
- Line 73: The commented-out log in D_FastLEDAudio.h within the
onVocalConfidence handler uses the integer format "%d" for the float variable
confidence; update the format specifier to "%f" (or a more specific float format
like "%.2f") in the log call (EXT_LOGD(ML_TAG, ...)) so that when uncommented it
prints the float value correctly.
In `@src/MoonLight/Nodes/Drivers/D_IMU.h`:
- Line 41: The update handler is registered with allowRemove=false which
prevents removeUpdateHandler from actually removing it in the destructor; update
the call site where ioUpdateHandler is assigned
(moduleIO->addUpdateHandler([this](const String& originId) { ... }, false)) to
pass true for the allowRemove parameter so the handler can be removed by
removeUpdateHandler in the destructor (ensure the same symbol names
ioUpdateHandler, moduleIO->addUpdateHandler and removeUpdateHandler are used to
locate the change).
In `@src/MoonLight/Nodes/Drivers/D_Infrared.h`:
- Line 108: The ioUpdateHandler is registered with addUpdateHandler(..., false)
so removeUpdateHandler becomes a no-op in the destructor; change the
registration in D_Infrared (the ioUpdateHandler lambda that calls readPins) to
allow removal (pass true or omit the explicit false) so removeUpdateHandler in
the class destructor actually unregisters the handler; references:
ioUpdateHandler, addUpdateHandler, removeUpdateHandler, readPins.
---
Nitpick comments:
In `@platformio.ini`:
- Line 204: The DRIVERS_STACK_SIZE was increased to 6144 as a workaround for
audio processing in the drivers task; instead create a dedicated audio FreeRTOS
task to isolate I2S audio reads and audioProcessor.update() from the drivers/LED
work so you can restore DRIVERS_STACK_SIZE to 4096. Implement a new task (e.g.,
audioTask) with its own stack and lower priority than the drivers/LED task, move
the I2S read loop and calls to audioProcessor.update() into that task, and
ensure safe data handoff to the drivers (use a thread-safe queue or ring buffer)
so DMA/timing-sensitive FastLED operations remain in the original drivers task.
In `@src/MoonLight/Nodes/Drivers/D_FastLEDAudio.h`:
- Around line 199-202: The destructor ~FastLEDAudioDriver currently calls
stopAudio() before moduleIO->removeUpdateHandler(ioUpdateHandler), which leaves
a race where moduleIO may invoke readPins() and call startAudio() on a
partially-destroyed object; fix by removing the update handler first (call
moduleIO->removeUpdateHandler(ioUpdateHandler) before stopAudio()) so no IO
callbacks can run while shutdown/stopAudio() is executing and ensure
ioUpdateHandler and moduleIO references are safe to use during removal.
In `@src/MoonLight/Nodes/Drivers/D_FastLEDDriver.h`:
- Around line 25-26: The two lifecycle token members ioUpdateHandler and
controlUpdateHandler in class D_FastLEDDriver are currently public but should be
private; move or relocate update_handler_id_t ioUpdateHandler and
update_handler_id_t controlUpdateHandler into the private section of
D_FastLEDDriver (or add a private: label above them) so they are not exposed in
the public interface, leaving other members and methods unchanged.
- Around line 66-74: The ioUpdateHandler currently only logs and must be
implemented to tear down and recreate FastLED channels when I2S pins change:
inside the lambda (ioUpdateHandler) delete existing channel objects/resources
the same way the class destructor does, then recreate channels and their configs
using the same logic as onLayout (respecting layerP.nrOfLedPins and
layerP.nrOfAssignedPins / MIN(...) used in the diff) so the new pin assignments
take effect; also detect and apply any maxPower changes to per-channel config
while recreating. Ensure you use the same helper routines or code paths used by
the destructor/onLayout for channel creation and deletion so behavior remains
consistent.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
platformio.inisrc/MoonBase/Nodes.hsrc/MoonLight/Nodes/Drivers/D_FastLEDAudio.hsrc/MoonLight/Nodes/Drivers/D_FastLEDDriver.hsrc/MoonLight/Nodes/Drivers/D_IMU.hsrc/MoonLight/Nodes/Drivers/D_Infrared.hsrc/MoonLight/Nodes/Effects/E_FastLED.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/MoonLight/Nodes/Effects/E_FastLED.h
| } | ||
| }; | ||
|
|
||
| ~IRDriver() { moduleIO->removeUpdateHandler(ioUpdateHandler); } |
There was a problem hiding this comment.
Missing override and no RMT hardware cleanup in destructor.
Two issues in the newly added destructor:
-
Missing
override: all other new destructors in this PR (~FastLEDDriver(),~FastLEDAudioDriver(),~IMUDriver()) useoverride; this one doesn't, which is inconsistent and suppresses compiler enforcement. -
Resource leak:
rx_channel(RMT channel) andreceive_queue(FreeRTOS queue) are allocated inreadPins()but never released when the driver is destroyed. The RMT peripheral will remain active after the node is removed.
🛡️ Proposed fix
- ~IRDriver() { moduleIO->removeUpdateHandler(ioUpdateHandler); }
+ ~IRDriver() override {
+ moduleIO->removeUpdateHandler(ioUpdateHandler);
+ if (rx_channel) {
+ rmt_rx_register_event_callbacks(rx_channel, &cbs_empty, receive_queue);
+ rmt_disable(rx_channel);
+ rmt_del_channel(rx_channel);
+ rx_channel = NULL;
+ }
+ if (receive_queue) {
+ vQueueDelete(receive_queue);
+ receive_queue = NULL;
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/MoonLight/Nodes/Drivers/D_Infrared.h` at line 434, The IRDriver
destructor currently lacks the override specifier and doesn't free RMT/RTOS
resources allocated in readPins(); update ~IRDriver() to be "~IRDriver()
override" and, in addition to moduleIO->removeUpdateHandler(ioUpdateHandler),
check if rx_channel and receive_queue are valid and properly tear them down
(call the appropriate RMT uninstall/stop API for rx_channel and delete the
FreeRTOS queue for receive_queue), then null out those members; ensure you
reference the same member names (rx_channel, receive_queue, ioUpdateHandler) and
the allocation site readPins() so the cleanup mirrors allocation.
Collaborator
Author
|
@coderabbitai, please review updatePin() in latest commit, this is a lightweight version of your proposal to. simplify readPins, see also its usage in FastLEDAudio.readPins(). does this handle full pin lifecycle: creating I2S pins in the IO module, changing them, deleting (part) of them? Is this a good candidate to use in all readPins()? I want to move updatePin() to a better class so it can be reused both by modules and nodes implementing readPins, where to move to? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 11594
✅ What Works WellYour
Usage is elegant: bool changed = updatePin(pinI2SWS, pin_I2S_WS);
changed = updatePin(pinI2SSD, pin_I2S_SD) || changed;
changed = updatePin(pinI2SSCK, pin_I2S_SCK) || changed;
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Chores